-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Throw Error When Using Unsupported Linux ARM #14616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
I think this should be sufficient - but let me know if I misunderstood the decision y'all made in Issue #13793 . Used System.getProperty() instead of current.Is() since ARM is part of the system architecture. |
|
the weird git diff error in the workflow should be fixed now - for some reason the linux word got removed, I put it back |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
@pujagani Failing test is unrelated:
|
|
looks like the same test is failing @pujagani
Seems like a good thing though? |
|
Yes, the test is not a concern. Thank you @shbenzer! |
| folder = "macos"; | ||
| } else if (current.is(LINUX)) { | ||
| folder = "linux"; | ||
| if (System.getProperty("os.arch").contains("arm")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also check for aarch64, see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create an issue and we’d be happy to take a look - commenting here will cause us to miss or forget @mkurz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I referenced the PR I created. I just wanted to make the author of the PR aware that the check was not enough. (Also for readers who stumple on this issue later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see - I didn't understand the link clearly on my phone. Thank you!
User description
Added code for Issue #13793 in Selenium Manager
Description
Selenium Manager Java should now throw an error if using unsupported Linux ARM architecture
Motivation and Context
Issue #13793
Types of changes
Checklist
PR Type
Bug fix
Description
SeleniumManagerto throw an exception when an unsupported Linux ARM64 architecture is detected.Changes walkthrough 📝
SeleniumManager.java
Add error handling for unsupported Linux ARM architecturejava/src/org/openqa/selenium/manager/SeleniumManager.java
WebDriverExceptionif ARM64 architecture is detected.